-
Notifications
You must be signed in to change notification settings - Fork 16
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
React 19 #3398
React 19 #3398
Conversation
Documentation has been published to https://lundalogik.github.io/lime-elements/versions/PR-3398/ |
19dc4cf
to
4d3328b
Compare
It's no longer needed when using React 19 which has much better support for rendering custom elements. This is a fix, because LimeElementsAdapter set some properties later depending on their type, meaning they were set later than expected in some situations or in unexpected order.
…d deps" This reverts commit 81e5a24.
events?: { [key: string]: (event: any) => void }; | ||
events: { change: (event: CustomEvent) => void }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm… this changes the object from allowing any number of properties ([key: string]
), to only allowing a single property called event
. Was that really intentional?
How about the following?
events: {
change: (event: CustomEvent) => void,
[key: string]: (event: any) => void,
};
This makes the object required, and it has to have the change
property, but it can also have any number of extra properties.
Although, come to think of it, this commit doesn't change lime-elements.api.md
, so I guess this is a purely internal change anyway?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was that really intentional?
Yes. That’s how it’s actually used, and if we need to handle more events, we can add them. It’s clearer what events need to be handled and easier to implement than allowing any event.
so I guess this is a purely internal change anyway?
Correct
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems to work flawlessly… 😅
Nice work! 💪
🎉 This PR is included in version 37.79.2 🎉 The release is available on: Your semantic-release bot 📦🚀 |
LimeElementsAdapter
that's used for rendering custom elements in rjsf formsThis is a fix, because LimeElementsAdapter set some properties later depending on their type, meaning they were set later than expected in some situations or in unexpected order.
Review:
Browsers tested:
(Check any that applies, it's ok to leave boxes unchecked if testing something didn't seem relevant.)
Windows:
Linux:
macOS:
Mobile: